Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport update to elastic-agent-go-sysinfo #40507

Conversation

fearful-symmetry
Copy link
Contributor

Proposed commit message

See #40491

Updates elastic-agent-system-metrics to v0.11.1 to remove an erroneous log line.

go get was insistent on updating go-sysinfo as well.

@fearful-symmetry fearful-symmetry self-assigned this Aug 13, 2024
@fearful-symmetry fearful-symmetry requested a review from a team as a code owner August 13, 2024 16:59
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Aug 13, 2024
@cmacknz
Copy link
Member

cmacknz commented Aug 13, 2024

Looks like the merge ate the changelog.

@andrewkroh
Copy link
Member

In effect, this is also backporting #40324 b/c of the sysinfo version. Just checking this is desired in a patch?

@fearful-symmetry
Copy link
Contributor Author

hmm, good point. @AndersonQ is that safe to backport?

@fearful-symmetry
Copy link
Contributor Author

Sigh. So elastic/elastic-agent-system-metrics#163 introduced a breaking change which was fixed in main by #40324, so if we don't want to backport that PR, we'll need to manually make some of the changes here.

@AndersonQ
Copy link
Member

hmm, good point. @AndersonQ is that safe to backport?

yes, it does have a breaking change, but it's reverting a breaking change.

@AndersonQ
Copy link
Member

Sigh. So elastic/elastic-agent-system-metrics#163 introduced a breaking change which was fixed in main by #40324, so if we don't want to backport that PR, we'll need to manually make some of the changes here.

I don't see a problem backporting them. Are you sure you linked the right system-metrics PR?

@AndersonQ
Copy link
Member

We could backport #40324 or just add the changelog form it to this backport

@fearful-symmetry
Copy link
Contributor Author

@AndersonQ can you backport that PR, if it's not too much trouble?

@AndersonQ
Copy link
Member

of course

@AndersonQ
Copy link
Member

@fearful-symmetry fearful-symmetry requested a review from a team as a code owner August 21, 2024 14:17
@fearful-symmetry fearful-symmetry requested review from AndersonQ and mauri870 and removed request for a team August 21, 2024 14:17
@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Aug 21, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Aug 21, 2024
@fearful-symmetry
Copy link
Contributor Author

fearful-symmetry commented Aug 21, 2024

@cmacknz so, this is a mess. In addition to the changes we discussed previously, there's also elastic/elastic-agent-system-metrics#172, which also introduced a ton of breaking changes, and I don't think we'd want to backport all of that to 8.15 as well.

I think we should just make a new branch in elastic-agent-system metrics that's around the tag of v0.10.3, backport the logging fix to that branch, and point the 8.15 branch of beats at that, or perhaps some other branch/release related fix, for the sake of getting this fix out before the freeze for 8.15.1. We should also probably just tag the main of elastic-agent-system-metrics as v1.0, so we force ourselves to have some kind of API guarantees, so this doesn't happen again.

@AndersonQ
Copy link
Member

We should also probably just tag the main of elastic-agent-system-metrics as v1.0, so we force ourselves to have some kind of API guarantees, so this doesn't happen again.

Agreed. I confess I think we should keep a v0 unless it's under active development and we do not know it's final form yet.

@fearful-symmetry I opened a PR to allow to set the hostname/fqdn to be lowercase. I'm using globals, well, because it allows to change the behaviour without changing the API, a quick fix to this mess:

fearful-symmetry added a commit to elastic/elastic-agent-system-metrics that referenced this pull request Aug 23, 2024
## What does this PR do?

So, a previous PR changed the behavior of the process code such that
we're now returning non-fatal errors.
However, a bunch of the tests were not updated, and are now failing. 

I'm a little worried by the fact that none of the tests failed
initially, as they fail when I run them locally. Part of the problem is
that the CI environment often doesn't have any processes running as
other users, meaning we won't hit a lot of code that deals with
permissions errors. Still, it feels like _something_ should have failed
here. Still investigating.

## Why is it important?

This is causing tests here to fail:
elastic/beats#40507

## Checklist

- [x] My code follows the style guidelines of this project
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have added an entry in `CHANGELOG.md`
Copy link
Contributor

mergify bot commented Aug 27, 2024

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b system-metrics-upgrade-11.1-backport upstream/system-metrics-upgrade-11.1-backport
git merge upstream/8.15
git push upstream system-metrics-upgrade-11.1-backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants